Skip to content

tests for cl_khr_spirv_queries #2409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Jun 5, 2025

@@ -0,0 +1,312 @@
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the SPIRV-Headers become a hard dependency, maybe we should just generate this file at build time?

Since cl_khr_spirv_queries support is now in the upstream headers,
we can use the defines from the headers vs. duplicating them in
the tests.
@karolherbst
Copy link
Contributor

looks good, tested my implementation of cl_khr_spirv_queries against this PR and everything passes.

Though I think if cl_khr_spirv_extended_debug_info is supported you could also check if OpenCL.DebugInfo.100 is.

@bashbaug
Copy link
Contributor Author

Though I think if cl_khr_spirv_extended_debug_info is supported you could also check if OpenCL.DebugInfo.100 is.

Good catch. I added this, although I don't have a device to test it with, and according to opencl.gpuinfo.org there aren't any devices in the database that support it, either, hrm.

As an aside, the tests are currently (informationally) printing any non-required SPIR-V capabilities that are reported. Should I do something similar for SPIR-V extended instruction sets and SPIR-V extensions also? This might help to find other missing checks like this one... or it could just be noise, once usage of the SPIR-V queries becomes more widespread.

@karolherbst
Copy link
Contributor

Though I think if cl_khr_spirv_extended_debug_info is supported you could also check if OpenCL.DebugInfo.100 is.

Good catch. I added this, although I don't have a device to test it with, and according to opencl.gpuinfo.org there aren't any devices in the database that support it, either, hrm.

don't have tests here for that either.

As an aside, the tests are currently (informationally) printing any non-required SPIR-V capabilities that are reported. Should I do something similar for SPIR-V extended instruction sets and SPIR-V extensions also? This might help to find other missing checks like this one... or it could just be noise, once usage of the SPIR-V queries becomes more widespread.

I think with this extension in mind it's fine to report more capabilities or extensions than necessary. They can also be part of vendor/ext extensions the tests here isn't aware off, so I'm pretty sure it's gonna cause some noise sooner or later.

@karolherbst
Copy link
Contributor

karolherbst commented Jun 12, 2025

need to add "cl_khr_spirv_queries" inside test_conformance/compiler/test_compiler_defines_for_extensions.cpp

otherwise test_compiler compiler_defines_for_extensions will fail: "FAIL: Extension cl_khr_spirv_queries is not in the list of approved Khronos extensions!"

This change could go on its own, but I think it's fine adding it here if this PR gets merged soonish.

@bashbaug
Copy link
Contributor Author

need to add "cl_khr_spirv_queries" inside test_conformance/compiler/test_compiler_defines_for_extensions.cpp

Fixed.

I really want to get that test fixed; it's kind of silly how we need to modify it every time a new KHR extension gets published.

@karolherbst
Copy link
Contributor

need to add "cl_khr_spirv_queries" inside test_conformance/compiler/test_compiler_defines_for_extensions.cpp

Fixed.

I really want to get that test fixed; it's kind of silly how we need to modify it every time a new KHR extension gets published.

yeah.. this list really should be generated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants